x11: Prevent memory corruption in XrmSetDatabase #1291
Conversation
Despite what the XrmSetDatabase docs say, it may try to free the database, resulting in memory corruption. Prevent that by making sure it has no db to act on. If it's past the first run, we will have done XrmDestroyDatabase above anyway. This causes a segfault, reported in issue dunst-project#1256. From the XrmSetDatabase man page: > The database previously associated with the display (if any) is not > destroyed. But this is patently false, from the source in Xlib's src/Xrm.c: void XrmSetDatabase( Display *display, XrmDatabase database) { LockDisplay(display); /* destroy database if set up implicitly by XGetDefault() */ if (display->db && (display->flags & XlibDisplayDfltRMDB)) { XrmDestroyDatabase(display->db); display->flags &= ~XlibDisplayDfltRMDB; } display->db = database; UnlockDisplay(display); } This is broken in Xlib since at least 2004[0], so now it's unfortunately a backwards compatibility issue... Fixes dunst-project#1256. 0: https://lists.freedesktop.org/archives/xorg-commit-diffs/2004-March/000239.html
|
Conditions for repro are:
|
|
I was curious why others haven't run into this, and found out they have, for example this block in rxvt-unicode: https://github.com/exg/rxvt-unicode/blob/5ec6fb5ec7f0871f19814279575a2f7873e5556a/src/rxvttoolkit.C#L542-L543 |
|
Many thanks. Do you know why in the rxvt code that line is protected by a |
|
The flag is enabled by autotools if the following compiles: What's happened here is that the first illegal accesses they found were in xdefaults. From /* Get any Xserver defaults */
if (refresh)
{
// fucking xlib keeps a copy of the rm string
Atom actual_type;
int actual_format;
unsigned long nitems, nremaining;
char *val = 0;
#if XLIB_ILLEGAL_ACCESS
if (dpy->xdefaults)
XFree (dpy->xdefaults);
#endif
[...]They were concerned that since modifying this is not permitted by Xlib's formal API, it should be tested that it at least compiles. Then further uses of XLIB_ILLEGAL_ACCESS were extended to other functions with broken behaviour, but the compile test was never updated to also test whether those compile (although they clearly always do, since nobody has ever complained about it). Considering the compile test will always pass on any existing Xlib -- I looked back a long time and the relevant parts of these structs have not changed for at least 20 years) -- |
|
Thanks for the explanation. Your solution seems reasonable to me so I'll merge 👍🏻 |
|
Thank you! Much appreciated. |
2 participants
Despite what the XrmSetDatabase docs say, it may try to free the database, resulting in memory corruption. Prevent that by making sure it has no db to act on. If it's past the first run, we will have done XrmDestroyDatabase above anyway. This causes a segfault, reported in issue #1256.
From the XrmSetDatabase man page:
But this is patently false, from the source in Xlib's src/Xrm.c:
This is broken in Xlib since at least 2004[0], so now it's unfortunately a backwards compatibility issue...
Fixes #1256.
0: https://lists.freedesktop.org/archives/xorg-commit-diffs/2004-March/000239.html